-
-
Notifications
You must be signed in to change notification settings - Fork 17
Adds enhanced search #287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Adds enhanced search #287
Conversation
let path = (page || ("api/v3/search/?q=" + projstr + "+" + encodeURIComponent(str))); | ||
if(debug) { | ||
url = "https://corsproxy.io/?https://readthedocs.org/" + path; | ||
}else{ | ||
url = "/_/" + path; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slowe I did bad things to try and get the RTD preview working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I broke "load more" in the process 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I fixed it for load more, but I made it much more ugly in the process!
@slowe here's some things I noticed while testing with the RTD preview:
|
First thing to note: I'm only able to test on a local file system so I may not have the same situation as you.
This is probably down to the change to hide empty tabs. I'll have a look.
Can you give an example of when? I am not seeing the same thing so it is probably down to a difference in expectation.
By "hidden" do you mean "not loaded"?
Please can you advise on what that should be? |
To clarify what I think is happening around the "Load more" button... I make one request to the ReadTheDocs API that includes all the projects. ReadTheDocs then returns up to 50 results at a time e.g. https://readthedocs.org/api/v3/search/?q=project:sunpy+project:ndcube+project:sunraster+project:aiapy+fits I'm then doing the totalling up by project. It is only possible to know the count by project for the results that have been loaded so far. So there may only be, say, "4" results loaded in a particular project but that doesn't mean there couldn't be more in the next batch of 50 results. The "Load more" button should keep appearing until all the results have been loaded. An alternative would be to make n requests to ReadTheDocs, one for each project every time the typeahead activates. This is inefficient and, potentially, acts more like a denial-of-service attack. Is it worth the number of requests to remove a slight, potential, confusion? I would also hope that most people have found what they were looking for within the first 50. |
The styling on the "Load more" button is the same as the styling on the buttons on the front of sunpy.org as it uses the classes |
Ah ha. This is how it looks for me locally: And this is on the "website preview" link (set to But what I said about the styles is because the classes are set in
I didn't add |
Ah ok, that makes sense. It just surprised me that I could see a (4) for a project followed by load more. I assume that if there are no more pages of results to load for any project the load more wont show?
Ah I see, I think we can safely add them by default they are defined in this theme. (Which I see you have done.) @slowe Could you have a look over the changes I made to the cors proxy code if you haven't already? I think there's a couple of things left to do before we merge this, but I think they are mostly on me:
|
A more compact version of the URL building. Remove/define some variables.
Switching to classes `sd-btn sd-bg-primary sd-bg-text-primary` from https://sunpy--287.org.readthedocs.build/projects/sunpy-sphinx-theme/287/web-components.html
I've changed the default "Load more" button styles to
That's the case, yes. It basically depends on the existence (or not) of a Would it help if the icon in each project tab was a filter icon (e.g. https://icons.getbootstrap.com/icons/funnel-fill/) to visually distinguish those tabs from the "All" tab? |
Yeah, I think that's a good idea. |
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
I fixed it. |
# Enable project-wide search with RTD | ||
rtd_search = True | ||
rtd_search_projects = | ||
rtd_search_load_more_label = | ||
rtd_search_no_results_label = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these explained anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds enhanced search functionality using ReadTheDocs API to enable cross-project search with inline results. The implementation provides a rich interface with tabbed results, keyboard navigation, and fallback to standard search when CORS is not enabled.
Key changes:
- New RTD-powered search interface with inline results and tabbed project filtering
- JavaScript and CSS files to implement the enhanced search experience
- Theme configuration options for customizing search behavior
- Integration with existing navbar links to automatically detect searchable projects
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
theme.conf | Adds configuration options for RTD search functionality |
rtd_enhanced_search.js | Core JavaScript implementation for enhanced search interface |
rtd_enhanced_search.css | Styling for the enhanced search modal and results |
cards.py | Updates CSS file path reference |
init.py | Integrates enhanced search into theme setup and configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
raise ValueError(err) | ||
return out_links | ||
|
||
projects = filter_doc_links(doc_links) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable projects
is used but only defined inside the conditional block when search_projects
is None. If search_projects
is not None, projects
will be undefined, causing a NameError when used in the JSON serialization on line 194.
projects = filter_doc_links(doc_links) | |
projects = filter_doc_links(doc_links) | |
else: | |
projects = search_projects |
Copilot uses AI. Check for mistakes.
function highlightAllMatches(text, search) { | ||
const regex = new RegExp(`(${search})`, "gi"); | ||
return text.replace(regex, "<strong>$1</strong>"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function highlightAllMatches
is defined but never used in the code. Consider removing it if it's not needed, or implement its usage if it was intended to be part of the functionality.
function highlightAllMatches(text, search) { | |
const regex = new RegExp(`(${search})`, "gi"); | |
return text.replace(regex, "<strong>$1</strong>"); | |
} |
Copilot uses AI. Check for mistakes.
This adds a first draft of an enhanced search interface that uses ReadTheDocs's API to search across multiple projects as requested by @Cadair.
This uses the existing search feature but then adapts the interface to add inline results rather than redirect to the search results page. If CORS is not enabled (with ReadTheDocs) it will redirect to the standard site search page. Alternatively, you can add
?debug
to the URL to usehttps://corsproxy.io/
to get around CORS issues.The inline search results will be arranged into tabs showing "All" results and results broken down by project. By default, the script will attempt to find projects from the page. It will look for elements with
class="nav-link"
from a menu item withid="Documentation"
. However, you can manually over-ride this behaviour by using the Javascript variableset_search_config
e.g.where:
no-results
- an optional object to set the label that gets displayed when there are no results;load-more
- an optional object to set the class/label of the "load more" button;projects
- an ordered array of projects to include (if empty this will be constructed from the ".nav-link" items found within #Documentation). The order of this array sets the order of the search results tabs.